fix(jsonrpc): preserve absolute parse error columns - #4020
Conversation
427ba4b to
4ebcf73
Compare
RafaelGranza
left a comment
RafaelGranza
left a comment
There was a problem hiding this comment.
Thanks for your PR!
Please take a look at my comments, and after you address all of them, this work will nicely fix the open issue.
|
|
||
| func TestHandleParseErrorKeepsAbsoluteColumnForLongUnicodeLine(t *testing.T) { | ||
| server := jsonrpc.NewServer(1, log.NewNopZapLogger()) | ||
| req := `{"jsonrpc":"2.0","method":"x","padding":"` + strings.Repeat("👍", 160) + `","id":@}` | ||
|
|
||
| res, _, err := server.HandleReader(t.Context(), strings.NewReader(req)) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(res), `[line 1, position 209]`) | ||
| } |
There was a problem hiding this comment.
we can use TestHandleParseError to exercise this case.
RafaelGranza
left a comment
RafaelGranza
left a comment
There was a problem hiding this comment.
Please run BenchmarkHandle and BenchmarkHandleLargeRequest benchmarks from file jsonrpc/server_test.go, compare it to main, and post the numbers in the PR description.
| chunkSize int | ||
| position string | ||
| marker byte | ||
| }{ |
There was a problem hiding this comment.
I believe this is not needed.
Just adding entries to parseErrorTests would cover the new changes. And please cover the case when there are broken runes (part of it is no longer in the window).
If you find it is not possible, for any reason, please let me know, and lets move back to having 2 different tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4020 +/- ##
==========================================
- Coverage 79.19% 79.18% -0.02%
==========================================
Files 464 464
Lines 35741 35828 +87
==========================================
+ Hits 28305 28369 +64
- Misses 7427 7450 +23
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RafaelGranza
left a comment
RafaelGranza
left a comment
There was a problem hiding this comment.
Thanks again for you contribution, and also for addressing the previous comments.
I still have some comments I'd like you to give a look before proceeding with my review.
| line, col := lineAndColumn(c, markerPos) | ||
| msg := fmt.Sprintf("%s [line %d, position %d]", describeError(c.window, markerPos, err), line, col) | ||
| line, relativeCol, absoluteCol := lineAndColumn(c, markerPos) | ||
| msg := fmt.Sprintf("%s [line %d, position %d]", describeError(c.window, markerPos, err), line, absoluteCol) |
There was a problem hiding this comment.
Our linter is pointing this line is too long, breaking it in two should fix it.
| req: strings.Repeat(" ", 600) + "\n" + `{"padding":"` + strings.Repeat("👍", 160) + `","id":@` + strings.Repeat("x", 96) + `}`, | ||
| chunkSize: 127, | ||
| position: `[line 2, position 180]`, | ||
| marker: '@', |
There was a problem hiding this comment.
IMO we don't need this, we can use the same res field here.
The way we are doing with requireMarkerUnderByte is making the same marker position calculations as in prod and checking if they match. But this introduce a problem, how do we know they are not both equally wrong?
IMO, the right call would be not checking the internal behavior of pretty_error.go, just checking the error output is correct. How the computations are made internally don't matter that much.
If you are worried that the res field could get too large, just remember the printed message is capped by 512 bytes, so res is not that large.
There was a problem hiding this comment.
If you disagree with this approach, let me know.
But anyway, if you want to keep the chunkSize field, we should move to having two distinct tests, because TestHandleParseError increases a lot in complexity with these added fields.
2 participants
Description
JSON-RPC parse errors on lines longer than the 512-byte recovery window reported a column relative to the retained window instead of the original line. This tracks the discarded rune prefix while keeping the recovery buffer bounded.
The caret remains relative to the retained text, while the reported column is absolute. Window boundaries stay UTF-8-safe, and the counting path uses a no-newline fast path plus 64-bit continuation-byte masks to avoid per-rune decoding.
Fixes #3914.
Testing
go test ./jsonrpcgo vet ./jsonrpcBenchmarks
Compared upstream
mainat963f145with this PR atb87a330on Go 1.27.0 (windows/amd64), Ryzen 7 5700X,GOMAXPROCS=1:Five-run medians:
mainBenchmarkHandleBenchmarkHandleLargeRequest/1MBBenchmarkHandleLargeRequest/10MBThe five-run ranges overlap for all three benchmarks. Allocations remain unchanged for
BenchmarkHandle(24 allocs/op) and the 1 MB case (56 allocs/op).